GH-800: Drain the Pipeline #450

utkarshg6 · 2024-06-19T07:02:58Z

No description provided.

…_keys_for_terminal_response

…hutdown_signal

…nders

…rue passes for the first time

…n_signal_when_last_data_is_true

…wn_signal_when_last_data_is_true

…rue is working fine

…is passing

… gone

…e_received_by_stream_handler_pool

…lled_integration is passing

…rue is passing

…_streams()

…_up_dead_streams()" This reverts commit ae08930.

This reverts commit d81c2f0.

This reverts commit 1880006.

This reverts commit 4f8c141.

This reverts commit 31db7c4.

…ata_is_true is passing" This reverts commit 54040ae.

This reverts commit f94c8d3.

This reverts commit 3b387a7.

This reverts commit 2ecfebc.

…tical integration test passing

…ssage is improved

node/src/proxy_client/stream_reader.rs

dnwiebe · 2024-09-11T11:01:26Z

node/src/proxy_server/mod.rs

@@ -58,6 +58,8 @@ use tokio::prelude::Future;
 pub const CRASH_KEY: &str = "PROXYSERVER";
 pub const RETURN_ROUTE_TTL: Duration = Duration::from_secs(120);

+pub const STREAM_KEY_PURGE_DELAY: Duration = Duration::from_secs(5);


Think about this value. It's not only the time allowed for straggling packets to make it to their destination, it's also the time allowed for evil exit Nodes to pad connections with garbage that they'll get paid for. Remember, the browser has already shut this stream down, so the user will never see any of the straggling packets or their consequences. Therefore an evil exit Node, once it gets notification that a stream is to be shut down, can immediately start sending big packets full of random trash back to us, and we'll accept and pay for them for this long. This parameter needs fairly careful tuning for the sake of security.

A suggestion might be to put in logging logic (if it doesn't already exist) to log whenever we receive one of these straggler packets that we're going to throw away, and then the testers can provide us logs to look at so that we can see how late stragglers are in real life, and tune this parameter to a balance between getting banned as a deadbeat and paying for trash from evil exit Nodes.

dnwiebe · 2024-09-11T11:15:57Z

node/src/proxy_server/mod.rs

@@ -603,6 +633,10 @@ impl ProxyServer {
    }

    fn purge_stream_key(&mut self, stream_key: &StreamKey) {
+        debug!(


We should probably check here to see if the stream key has already been purged (by a previous delayed message) and abort if it has.

dnwiebe · 2024-09-11T11:20:58Z

node/src/proxy_server/mod.rs

+        debug!(
+            self.logger,
+            "Retiring stream key {}: no more data", &stream_key
+        );
        let _ = self.keys_and_addrs.remove_a(stream_key);
        let _ = self.stream_key_routes.remove(stream_key);
        let _ = self.tunneled_hosts.remove(stream_key);


Suggestion: Combine these last two HashMaps into one, with the StreamKey indexing a single structure that has both route and tunneled host in it. Add a third field that's a boolean flag that says whether the stream is in the process of waiting for stragglers. Use this flag in handle_client_response_payload() above to make sure each dying stream gets only one StreamKeyPurge message, no matter how many straggling packets come in for it.

Further suggestion: That flag that makes sure each StreamKey gets only one StreamKeyPurge message? Make it a struct that contains both that boolean flag and also two timestamps: one that carries the instant the terminal ExpiredCoresPackage was received, and one that carries the instant the most recent straggler was received. That way, when the StreamKeyPurge message is finally processed, you can create a log saying something like, "For StreamKey , stragglers kept arriving for milliseconds after it was ordered closed." That will help immensely with tuning the delay for the real world.

node/src/proxy_server/mod.rs

dnwiebe · 2024-09-11T11:29:19Z

node/src/proxy_server/mod.rs

+                delay: Duration::from_millis(stream_key_purge_delay_in_millis + offset_in_millis),
+            })
+            .unwrap();
+        system.run();


If this test fails, does the test panic, or does this system.run() just return an unsuccessful code? If the latter, this test will always pass. If you're absolutely sure that the test will panic if it fails, okay, but if it were me, I'd modify one of those assert!()s (maybe by adding or removing a !) to make sure the test fails and run it to see if I was right.

Also, this is a very clever test. I hope it's not too clever for folks to figure out. If you think it might be, a few judicious comments might help clarify it. Maybe a banner comment at the top with a little ASCII timeline diagram explaining what you're doing with those -100ms and +100ms messages.

It shows this once it fails: assertion failed: proxy_server.keys_and_addrs.is_empty().

node/tests/connection_shutdown_test.rs

node/src/proxy_client/stream_handler_pool.rs

utkarshg6 added 22 commits June 13, 2024 17:45

GH-800: Write the most basic TODOs for this card

4cfa253

GH-800: Add todos for renaming purposes

07cfef9

GH-800: Add a message scheduler for purging the stream key

62db40f

GH-800: Improve the test handle_client_response_payload_purges_stream…

e4199ae

…_keys_for_terminal_response

GH-800: Make delay configurable via constant

ee8f897

GH-800: introduce StreamSenders

91323d4

GH-800: introduce channel for shutdown signal

7cc2fbb

GH-800: add test connection_shutdown_test.rs

d784157

GH-800: test drive the shutdown signal in StreamReader

14a5de3

GH-800: refactor test stream_reader_shuts_down_when_it_receives_the_s…

d8d1f10

…hutdown_signal

GH-800: some refactor inside the file stream_reader

ddf7d9c

GH-800: some more refactoring

a6f2538

GH-800: trying_to_write_a_test_for_stream_senders was a success

130a37c

GH-800: did some refactoring for trying_to_write_a_test_for_stream_se…

6db4c0c

…nders

GH-800: stream_handler_pool_sends_shutdown_signal_when_last_data_is_t…

c3cc6a8

…rue passes for the first time

GH-800: Minor refactor for the test stream_handler_pool_sends_shutdow…

75edd8f

…n_signal_when_last_data_is_true

GH-800: more refactoring of the test stream_handler_pool_sends_shutdo…

296f449

…wn_signal_when_last_data_is_true

GH-800: stream_handler_pool_sends_shutdown_signal_when_last_data_is_t…

a2c763d

…rue is working fine

GH-800: some more todos removed

3c223ea

GH-800: clean_up_dead_streams_logs_when_the_shutdown_channel_is_down …

4605d83

…is passing

GH-800: tests are passing in StreamHandlerPool

e1417c8

GH-800: Wrote a test for the case when the shutdown signal channel is…

6e28ff1

… gone

utkarshg6 linked an issue Jul 22, 2024 that may be closed by this pull request

Drain the Pipeline MASQ-Project/MASQ-Node-issues#800

Open

utkarshg6 added 7 commits July 22, 2024 12:02

GH-800: make it easier to understand who experiences the write error

6849db6

GH-800: trying to write test while_housekeeping_the_stream_senders_ar…

af09d25

…e_received_by_stream_handler_pool

GH-800: add test add_new_streams_works

b896db6

GH-800: wip: add the log

acf2c89

GH-800: test proxy_client_stream_reader_dies_when_client_stream_is_ki…

deb1535

…lled_integration is passing

GH-800: change the channel from crossbeam to tokio

2ecfebc

GH-800: Add fn send_shutdown_signal_to_stream_reader

3b387a7

utkarshg6 added 2 commits August 7, 2024 17:55

GH-800: stream_handler_pool_sends_shutdown_signal_when_last_data_is_t…

54040ae

…rue is passing

GH-800: all tests in stream_handler_pool.rs are passing

31db7c4

kauri-hero closed this Aug 25, 2024

kauri-hero reopened this Aug 25, 2024

utkarshg6 added 19 commits August 27, 2024 12:44

GH-800: add test for the logs; all tests passing

4f8c141

GH-800: remove lookup_ip() mock fns from a test

1880006

GH-800: remove warnings

d81c2f0

GH-800: use fn send_shutdown_signal_to_stream_reader in clean_up_dead…

ae08930

…_streams()

Revert "GH-800: use fn send_shutdown_signal_to_stream_reader in clean…

23fc9ab

…_up_dead_streams()" This reverts commit ae08930.

Revert "GH-800: remove warnings"

f9491e7

This reverts commit d81c2f0.

Revert "GH-800: remove lookup_ip() mock fns from a test"

3e3fa71

This reverts commit 1880006.

Revert "GH-800: add test for the logs; all tests passing"

d4e8075

This reverts commit 4f8c141.

Revert "GH-800: all tests in stream_handler_pool.rs are passing"

5aa1107

This reverts commit 31db7c4.

Revert "GH-800: stream_handler_pool_sends_shutdown_signal_when_last_d…

f2dbe7b

…ata_is_true is passing" This reverts commit 54040ae.

Revert "GH-800: wip: fixing 2 tests"

a24dcda

This reverts commit f94c8d3.

Revert "GH-800: Add fn send_shutdown_signal_to_stream_reader"

c61cc5a

This reverts commit 3b387a7.

Revert "GH-800: change the channel from crossbeam to tokio"

af9a673

This reverts commit 2ecfebc.

GH-800: reverted the channel to crossbeam; remove warnings

5ba55df

GH-800: solve some TODOs

5579c12

GH-800: add refactored fn send_shutdown_signal_to_stream_reader; crti…

3f34e83

…tical integration test passing

GH-800: write_failure_for_nonexistent_stream_generates_termination_me…

681a79f

…ssage is improved

GH-800: everything is tested

0811b5e

GH-800: fix proxy_server_receives_terminal_response_from_hopper

d86918e

utkarshg6 marked this pull request as ready for review September 10, 2024 07:53

utkarshg6 and others added 5 commits September 10, 2024 13:24

GH-800: trigger actions

53eadfa

GH-800: assert on a different error on non-mac os

74eb857

Merge branch 'master' into GH-800

92d46cd

GH-80: add changes of self-review

ce0d3c5

GH-800: some other missed changes

7afe4c7

dnwiebe reviewed Sep 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-800: Drain the Pipeline #450

GH-800: Drain the Pipeline #450

utkarshg6 commented Jun 19, 2024

dnwiebe Sep 11, 2024

dnwiebe Sep 11, 2024

dnwiebe Sep 11, 2024

dnwiebe Sep 11, 2024

dnwiebe Sep 11, 2024

dnwiebe Sep 11, 2024

dnwiebe Sep 11, 2024

utkarshg6 Sep 12, 2024

GH-800: Drain the Pipeline #450

Are you sure you want to change the base?

GH-800: Drain the Pipeline #450

Conversation

utkarshg6 commented Jun 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment